fix(completion): a failed source cleanup is a failure, not a success - #9
Merged
Conversation
added 2 commits
August 22, 2026 19:11
…ccess After a delivery that was verified on every destination, a failure of the source completion action was logged and then ignored: the item was still persisted as `Completed`, `replicated` counted it, and a `FileArchived` event was emitted carrying a computed `archivePath` that pointed at a file which had never been written — while the source sat untouched in the watch directory. A missing `archiveDir` took the same path, silently degrading `onSuccess: archive` into "leave it where it is" and still reporting success. For an evidence pipeline that is a false record of custody. Completion is now proven before it is recorded. Two durable states carry it: `CleanupPending`, persisted before the filesystem is touched, and `CleanupFailed` when the action did not succeed. `Completed` is written only once the action is verified — the archive target exists at the source's byte count and, under `completion.verify: checksum`, re-hashes to the checksum the destinations verified against; or the deleted source is absent. A failed move, an unconfigured or unwritable `archiveDir`, a failed delete, and an archived copy that does not match are all cleanup failures: no `FileArchived`/`FileDeleted`, no `replicated` increment. `FileArchived.archivePath` now reports the path the file really landed at, which the `suffix` collision policy can rename. Cleanup retries run on their own bounded budget — the shared full-jitter backoff capped by `retry.maxAttempts`, else ten attempts — deliberately separate from the transfer's time-based `giveUpAfter`, whose clock starts at discovery and is usually spent by the time a slow transfer finishes. Permanent errors give up on the first attempt. Every reconciliation tick re-drives the `CleanupPending` rows and the due `CleanupFailed` rows; exhaustion parks the item with a `FileCleanupFailed` event and a `failed.items[]` entry in `get-status` (`state: "cleanup_failed"` plus `cleanupAttempts`), recoverable with `trigger`, which re-drives every cleanup failure regardless of its gate. Recovery re-evaluates a `CleanupPending` item against observed filesystem state before any new work: a source still present retries the action, a source already gone completes (archiving removes the source only after the target rename succeeds). Neither cleanup state is terminal, so a rescan that re-discovers the still-present source preserves the row instead of re-enqueueing an already-replicated file. Adds a `cleanup_attempts` column to `work_items`, applied to an existing database with a guarded `ALTER TABLE`, and a `SourceFs` seam in the worker so the failure paths are tested without a real cross-device mount. DESIGN.md gains register entry I, FR-CMP-7, the reworked §8.1 state machine and §13.2 completion sequence; the reference, explanation, and how-to docs describe the new behavior.
A source completion action that fails because something else still holds the file open was classified permanent and parked the item in `CleanupFailed` after a single attempt. On Windows that is the ordinary case — a producer finishing its write, an antivirus scanner, an indexer, a backup agent — and it clears on its own within seconds, so the file demanded operator action for a condition that heals itself. `ReplError::classify_cleanup_io` now classifies a locked file as transient on the cleanup path: `PermissionDenied` and `ResourceBusy` by `io::ErrorKind`, plus the platform's raw code — Windows `ERROR_SHARING_VIOLATION` (32), Unix `EBUSY` (16) — matched per platform because 32 is `EPIPE` on Unix. Both are checked because which of the two a lock surfaces as depends on the OS and the toolchain version. Everything else keeps `classify_io`'s rules, so `NotFound` still fails fast. The transfer path is unchanged: there a `PermissionDenied` is a credential or ACL an operator must fix, and failing fast to `Exhausted` is right. The shared `move_file` helper takes the classifier as a parameter so each caller keeps its own policy — the completion action passes `classify_cleanup_io`, quarantine passes `classify_io`. Adds a fault-injection test proving a locked source retries on the cleanup backoff (transient error recorded, no `FileCleanupFailed`, no `FileDeleted`, `replicated` unmoved) and then completes once the lock clears. The uncreatable-archive-dir test now asserts the retry before the give-up. Classifier unit tests cover both error kinds, the platform raw code, and that `NotFound`/`TimedOut` are unaffected. DESIGN §13.2 and register entry I record the divergence and why; the explanation page documents it alongside the cleanup retry budget.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Defect
After a verified replication, a failure of the source completion action (archive move or delete) was logged and the item was still persisted as
Completed,replicatedwas incremented, andFileArchivedwas emitted with a computedarchivePatheven though the move never happened — a false success for an evidence pipeline (worker.rsapply_success_actiononly warned; DESIGN §20-B). Raised byimage-processorD-IP-16.Fix
CleanupPending→Completed|CleanupFailed(→CleanupPendingon retry /trigger).CleanupPendingis persisted before the filesystem is touched;Completedonly after the action is proven (archive: target exists at the source's byte count and re-hashes to the delivered checksum undercompletion.verify = checksum; delete: source absent). A missing/uncreatablearchiveDiris a cleanup failure.FileArchived/FileDeletedfire only on a proven action with the resolved target path;replicatedcounts only onCompleted.retry.maxAttempts, else 10); exhaustion parks the item (next_attempt_at = i64::MAX) and emitsFileCleanupFailed {path, action, attempts, lastError}. Parked items are re-driven on every reconciliation tick (gated) and bytrigger(ungated).get-statuslists them underfailed.items[]withstateandcleanupAttempts.CleanupPendingrows against observed state before new work;CleanupPending/CleanupFailedsources are never re-enqueued as new work.PermissionDenied/ResourceBusy, WindowsERROR_SHARING_VIOLATION, UnixEBUSY) is transient and retried; the transfer path's classification is unchanged (classifier passed explicitly).work_items.cleanup_attemptscolumn added by a guardedALTER TABLEmigration.Docs:
DESIGN.md(§13 completion, §20 state machine + register entry I),AGENTS.md,docs/explanation.md,docs/how-to-guides.md,docs/reference/{configuration,data-types,messaging-interface}.md— current-state prose.Validation
cargo test: 415 passed (+ 30 across 11 integration binaries);cargo clippy --all-targets --features dest-s3,dest-sftp,dest-ftps,dest-http,dest-azure,dest-gcs -- -D warnings: clean.cargo llvm-cov --fail-under-lines 90: 94.22 %.triggerre-drive, locked source retries then completes); all recovery rules; instance tick/trigger wiring.lab-5950x, Kubernetes, and the Dallas full-system E2E. The change is platform-agnostic filesystem/state logic, butget-statusand theevtcatalog each gained a field/type, so a Dallas pass is the honest confirmation before release.